[FEDE-8281] Upgrade Arrow Java to siren-18.3.0-1#2
[FEDE-8281] Upgrade Arrow Java to siren-18.3.0-1#2GeorgeAp wants to merge 8 commits intobranch-siren-18.3.0-xfrom
Conversation
| } catch (NoSuchMethodException e) { | ||
| logger.debug("Cannot get constructor for direct buffer allocation", e); | ||
| return e; | ||
| } catch (SecurityException e) { |
There was a problem hiding this comment.
@torito this change was due to the [2021-11-24T15:22:37,545][DEBUG][s.i.n.u.i.PlatformDependent0] [mars] direct buffer constructor: unavailable reported in the PR sirensolutions/arrow#17.
The issue was widely discussed in google/gson#1875 and google/gson#1902
There was a problem hiding this comment.
This is the exception thrown if we revert our change
java.lang.NoClassDefFoundError: Could not initialize class org.apache.arrow.memory.RootAllocator
at __randomizedtesting.SeedInfo.seed([9E683B735110AFF2:76856F24D3CDD0A6]:0)
... federate calls
at java.base/java.lang.Thread.run(Thread.java:1583)
Caused by: java.lang.ExceptionInInitializerError: Exception java.lang.ExceptionInInitializerError [in thread "SUITE-UTFHelperTest-seed#[DF6017972AC13922]-worker"]
at org.apache.arrow.memory.unsafe.UnsafeAllocationManager.<clinit>(UnsafeAllocationManager.java:29)
at org.apache.arrow.memory.unsafe.DefaultAllocationManagerFactory.<clinit>(DefaultAllocationManagerFactory.java:26)
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:421)
at java.base/java.lang.Class.forName(Class.java:412)
at org.apache.arrow.memory.DefaultAllocationManagerOption.getFactory(DefaultAllocationManagerOption.java:105)
at org.apache.arrow.memory.DefaultAllocationManagerOption.getDefaultAllocationManagerFactory(DefaultAllocationManagerOption.java:92)
at org.apache.arrow.memory.BaseAllocator$Config.getAllocationManagerFactory(BaseAllocator.java:826)
There was a problem hiding this comment.
Those discussions on gson are not entirelly related, can you please:
- instead of generic
catch(Exception e), add a new catch (RealReceivedException e) {...} // <-- this to propose an upstream fix - Explain why if the exception is thrown, federate still works ? Direct Buffer is not needed for us ? what do we use instead ?
There was a problem hiding this comment.
The real exception is not InaccessibleObjectException ?
There was a problem hiding this comment.
It is actually that
java.lang.reflect.InaccessibleObjectException: Unable to make private java.nio.DirectByteBuffer(long,long) accessible: module java.base does not "opens java.nio" to unnamed module @2d6e8792
There was a problem hiding this comment.
can we add only a new catch for InaccessibleObjectException instead of a generic one
There was a problem hiding this comment.
I reverted our change and added it in-place of the SecurityException d6fca40, wdyt?
There was a problem hiding this comment.
I have added the SecurityException back
|
|
||
| /** The unsafe object from which to access the off-heap memory. */ | ||
| private static final Unsafe UNSAFE; | ||
| public static final Unsafe UNSAFE; |
There was a problem hiding this comment.
why we need those to be public ?, where we are accessing it in Federate ?
There was a problem hiding this comment.
We access it in Federate and keeping it private breaks our usage
There was a problem hiding this comment.
In federate we are already accessing the Unsafe, see https://github.com/sirensolutions/siren-platform/blob/f32ca909badbf0527ffe90231fa5b52ee36a3d35/core/src/main/java/io/siren/federate/core/common/ReflectionHelper.java#L129
So, instead of geting the Unsafe from MemoryUtil, we access it by using the one we get from the ReflectionHelper ? This to limiting dependency on the fork, wdyt ?
There was a problem hiding this comment.
Yes, good point, I will test this
There was a problem hiding this comment.
I have reverted the change to keep it private in 003619b
|
|
||
| /** The start offset of array data relative to the start address of the array object. */ | ||
| private static final long BYTE_ARRAY_BASE_OFFSET; | ||
| public static final long BYTE_ARRAY_BASE_OFFSET; |
There was a problem hiding this comment.
The same for this offset, it seems we are also getting this value in Federate by using reflection as it is done here
…ce we have access to them in Federate through the ReflectionHelper
|
|
||
| // get the offset of the address field in a java.nio.Buffer object | ||
| Field addressField = java.nio.Buffer.class.getDeclaredField("address"); | ||
| addressField.setAccessible(true); |
There was a problem hiding this comment.
This line should be removed also ? this code block is throwing an exception any way, maybe even before reaching this line, so maybe we dont need to remove it
There was a problem hiding this comment.
I will validate this, the exception is currently thrown at L103 constructor.setAccessible(true);
What's Changed
Upgrade Arrow Java to 18.3..0
Change logs from Arrow 16.0.0 to Arrow Java 18.3.0
Arrow 16.0.0 - https://arrow.apache.org/blog/2024/04/20/16.0.0-release/
Arrow 17.0.0 - https://arrow.apache.org/blog/2024/07/16/17.0.0-release/
Arrow 18.0.0 - https://arrow.apache.org/blog/2024/10/28/18.0.0-release/
Arrow-Java 18.2.0 to 18.3.0 - https://github.com/apache/arrow-java/releases
Please fill in a description of the changes here.
This contains breaking changes.
Closes https://sirensolutions.atlassian.net/browse/FEDE-8281